- 
                Notifications
    
You must be signed in to change notification settings  - Fork 98
 
Fix support for Xcode 9 -> 10.2 #363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1b836b0    to
    e6fb27a      
    Compare
  
    e6fb27a    to
    943311b      
    Compare
  
    ea249bc    to
    b27503c      
    Compare
  
    b27503c    to
    b8e0c86      
    Compare
  
    | XCTAssertNotNil(task) | ||
| 
               | 
          ||
| waitForExpectations(timeout: 2) { (error) in | ||
| waitForExpectations(timeout: 5) { (error) in | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even locally, 2 seconds did timeout 1/16 times for the 3.2 MB GeoJSON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, somehow I didn’t realize our test fixtures were that big. This isn’t a performance test, so we should probably think about breaking this test case up into minimal test cases the next time we need to fix it.
21ef2c6    to
    639dda0      
    Compare
  
    639dda0    to
    021348b      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested changelog entry:
- Fixed compatibility issues with Xcode 10.2 when the SDK is installed using Carthage. (#363)
 
| @@ -1,2 +1,2 @@ | |||
| binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 4.4 | |||
| github "AliSoftware/OHHTTPStubs" ~> 6.0 | |||
| github "linksmt/OHHTTPStubs" "563f48d3fab84ef04639649c770b00f4fa502cca" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a PR for this change or something else we can keep track of for when master gets updated? AliSoftware/OHHTTPStubs#295 is already in v7.0.0; is this for something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, looks like this commit also pulls in AliSoftware/OHHTTPStubs#286. The CircleCI configuration isn’t testing on watchOS anyways. So I think we can pin to the main pod’s v7.x.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, the reason why I switched to the fork here is the missing watchOS scheme in the original. We don't test on watchOS but we do compile for watchOS. We could switch back but it would require a workaround to run carthage bootstrap and ignore the .private and .resolved files.
| XCTAssertNotNil(task) | ||
| 
               | 
          ||
| waitForExpectations(timeout: 2) { (error) in | ||
| waitForExpectations(timeout: 5) { (error) in | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, somehow I didn’t realize our test fixtures were that big. This isn’t a performance test, so we should probably think about breaking this test case up into minimal test cases the next time we need to fix it.
Upgraded to CircleCI 2.1
Running tests on
Xcode 9.4.1 iOS 9.3
Xcode 10.0 iOS 12.0
Xcode 10.2 iOS 12.2
Despite the branch name, this PR doesn't migrate to Swift 5, it merely validates compatibility with various versions of Xcode.
cc @1ec5